fix(cmd): exit non-zero when a command handler returns an error (#4255) - #4258
NitinKumar004 wants to merge 3 commits into
Conversation
…-dev#4255) CMD apps exited 0 even when the sub-command handler (or command resolution) returned an error, so shells and CI could not detect the failure. Responder now records whether it responded with an error; cmd.Run propagates that, and runCMD exits with a non-zero status after telemetry is flushed and the logger is closed, so a failed command still reports its final metrics/traces. os.Exit is wrapped in a testable seam.
The non-zero exit was terminating in-process tests that invoke Run (including apps' own main() tests, e.g. examples/sample-cmd). Guard the exit with testing.Testing() so real CLI binaries still exit non-zero while go test runs are not killed. Extract the telemetry flush so its deferred cancel runs before the exit, and cover cmd.Run's error/success/unknown-command return value.
aryanmehrotra
left a comment
aryanmehrotra
left a comment
There was a problem hiding this comment.
Thanks for this — the behaviour is right and the exit-code matrix is correct. I built a CMD app with 8 subcommands and drove it out-of-process, reading ExitError.ExitCode():
| case | development |
this PR |
|---|---|---|
| handler returns nil | 0 | 0 |
| handler returns error | 0 (the bug) | 1 |
| data and error | 0 | 1 (stdout still written) |
| unknown / missing subcommand | 0 | 1 |
--help |
0 | 0 |
| stderr written, nil error | 0 | 0 |
The last row is the one I most wanted to check and you got it right — keying off the returned error rather than stderr output is correct, and a naive implementation would have conflated them.
The flush ordering is careful work: extracting flushCMDTelemetry so its deferred cancel runs before the exit is a subtlety that's easy to miss, and I confirmed the final ERROR line reaches CMD_LOGS_FILE on the failure path.
Requesting changes on one thing.
testing.Testing() makes the shipped behaviour untestable by construction
if failed && !testing.Testing() {
os.Exit(1)
}I deleted os.Exit(1) from run.go:41 entirely and the full pkg/gofr suite still passed (ok gofr.dev/pkg/gofr 7.056s). Coverage agrees — run.go:39.34,42.3 has 0 hits. So the branch that defines this PR's whole contract is the one branch no test can reach.
Two consequences beyond that:
- A user who writes an in-process test asserting their CLI fails is told it succeeded.
- A CLI driven from a
go test -cbinary gets a different process contract than the same code shipped.
Your original var osExit = os.Exit (a6665d3bc) was the right shape, and e02aca3da removed it along with run_test.go. Could we bring the seam back and solve examples/sample-cmd's two failure-path tests another way — either by having them set the seam, or by running the binary out-of-process for those two cases?
Worth knowing: #3942 adds an exit func(int) field on App plus exitCodeStartupFailed = 1 and abortStartup(), for exactly this reason, about 30 lines away in the same file. I test-merged the two — no textual conflict, builds clean, tests pass — so this will land silently and leave run.go holding two different exit mechanisms. If #3942 goes first this becomes a two-line change and the testing import, the guard and the second //nolint all disappear.
The description no longer matches the code
The body says "os.Exit is wrapped in a small testable seam (osExit)", but grep -rn osExit on the branch returns nothing. Please update it — a reviewer reading the description would approve a design that isn't there.
Smaller points
- User
defers inmain()are skipped on the failure path. Withdefer cleanup()as the first line ofmain,./app okruns it and./app faildoes not. Anyone doingdefer db.Close()ordefer pprof.StopCPUProfile()loses it exactly when things went wrong. Worth a sentence onrunCMD's doc comment. - The
//nolint:reviveargues the wrong thing. Its explanation ("exit status 1 signals the failed command to shells and CI") argues why code 1, not why a deep exit is acceptable here. I confirmed the lint genuinely fires without it (deep-exit: calls to os.Exit only in main() or init() functions).abortStartupin #3942 spends five lines on the exemption; same file, same rule. - Docs.
docs/advanced-guide/building-cli-applications/page.mdnever mentions exit codes. This is now a process contract that scripts and CI branch on — two lines there would help: nil → 0, error or unknown command → 1,--help→ 0. - Nit:
Errored()is an addition to the exportedpkg/gofr/cmdAPI, so "None to the public API" is slightly off. Additive and non-breaking, but worth stating accurately. - Nit:
os.Exit(1)is a bare literal here; #3942 introducesexitCodeStartupFailed = 1. Whichever lands second, these should share one constant.
Also checked and dismissed: importing testing adds no dependency — it's already a transitive import of pkg/gofr via service, datasource/sql and container. No binary-size concern there.
Verified: gofmt clean, go vet clean, golangci-lint has 0 issues in any changed file, all three new cmd_test.go tests kill their mutants, CI green on Go 1.24/1.25/1.26. The two local -race/-count=5 failures (websocket.go:47 race, port-2121 probe) reproduce identically on development.
…ing.Testing() The exit was guarded with !testing.Testing(), so no test could reach it: deleting os.Exit left the whole suite green. - Replace the guard with an App.exit func(int) seam (nil means os.Exit), the same field gofr-dev#3942 introduces, plus exitCodeCommandFailed = 1. The deep-exit nolint now explains why the exit is acceptable at that point. - TestApp_runCMD_exitCode covers nil error, error, data+error, unknown and missing command, and --help; deleting the exit call now fails it. - TestNewCMD pins os.Args and records the exit instead of relying on the guard. - examples/sample-cmd: the two failure-path tests run main() in a child copy of the test binary and assert exit code 1 plus stderr. `command help` (not a registered subcommand) moves to that table; it always wrote the not-a-valid-command error, and now also exits 1. - runCMD's doc comment and the CLI docs describe the exit codes and note that defers in main() do not run when a command fails.
|
Thanks for driving it out-of-process. You're right that the Pushed 2758da4:
|
2 participants
Description:
Fixes #4255.
CMD apps built with GoFr exited
0even when the sub-command handler (or command resolution) returned an error, so shells, pipelines and CI could not detect the failure.Changes:
(*cmd.Responder).Errored()reports whether the command responded with a non-nil error.cmd.Runreturns that to the caller. The decision is keyed off the returnederror, not off stderr output, so a handler that writes to stderr but returnsnilstill exits0.runCMDflushes telemetry and closes the logger first, then exits withexitCodeCommandFailed(1) if the command failed. The flush lives influshCMDTelemetryso its deferredcancelruns before the exit.exit func(int)field onApp: nil meansos.Exit, and tests set it to observe the status. It is the same field fix(mcp): claim the MCP port with net.Listen instead of exiting from EnableMCP #3942 adds, so whichever PR lands second resolves a one-line conflict inAppand can fold the two exit helpers and constants into one.Exit codes:
nil-h/--help, alone or after a registered subcommandNote: because the process exits via
os.Exiton failure, functions deferred in the user'smain()don't run in that case. This is documented inrunCMD's doc comment and indocs/advanced-guide/building-cli-applications.Tests:
TestApp_runCMD_exitCode, table-driven with an exit recorder, covers: nil error → no exit; error, data+error, unknown and missing command → exit 1;--help→ no exit. Deleting the exit call fails it.examples/sample-cmd: the two failure-path tests runmain()in a child copy of the test binary and assert exit code 1 and the exact stderr.command helpisn't a registered subcommand. It always printed the not-a-valid-command error, and it now also exits 1, so it moved into that table.Breaking Changes (if applicable):
No signature changes. The API additions are the exported
(*cmd.Responder).Errored()method (additive) and the unexportedApp.exitfield. The behavior change is the intended one: a CMD app now exits non-zero when a command fails.Additional Information:
No new dependencies.
Checklist:
goimportandgolangci-lint.